Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WithGivens for monoidal and (co)cartesian derivations #1494

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

WithGivens for monoidal and (co)cartesian derivations #1494

wants to merge 4 commits into from

Conversation

TKuh
Copy link
Collaborator

@TKuh TKuh commented Oct 17, 2023

Depends on #1492

Part of #1239

@codecov
Copy link

codecov bot commented Oct 17, 2023 •
edited
Loading

Codecov Report

Attention: 144 lines in your changes are missing coverage. Please review.

Comparison is base (0d99008) 80.89% compared to head (9698af4) 80.88%.

Files Patch % Lines
...recompiled_categories/MatrixCategoryPrecompiled.gi 54.76% 76 Missing ⚠️
...SymmetricClosedMonoidalCategoriesDerivedMethods.gi 67.92% 17 Missing ⚠️
...mmetricCoclosedMonoidalCategoriesDerivedMethods.gi 66.66% 17 Missing ⚠️
...ymmetricCartesianClosedCategoriesDerivedMethods.gi 73.58% 14 Missing ⚠️
...tricCocartesianCoclosedCategoriesDerivedMethods.gi 72.54% 14 Missing ⚠️
...es/gap/BraidedCartesianCategoriesDerivedMethods.gi 50.00% 2 Missing ⚠️
.../gap/BraidedCocartesianCategoriesDerivedMethods.gi 50.00% 2 Missing ⚠️
...ies/gap/BraidedMonoidalCategoriesDerivedMethods.gi 50.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #1494      +/-   ##
==========================================
- Coverage   80.89%   80.88%   -0.02%     
==========================================
  Files         487      487              
  Lines       62313    62369      +56     
==========================================
+ Hits        50409    50446      +37     
- Misses      11904    11923      +19
Flag Coverage Δ
ActionsForCAP 64.09% <ø> (ø)
AttributeCategoryForCAP 88.88% <ø> (ø)
CAP 84.47% <ø> (ø)
CartesianCategories 93.30% <71.68%> (-0.03%) ⬇️
CompilerForCAP 94.39% <ø> (ø)
ComplexesAndFilteredObjectsForCAP 73.60% <ø> (ø)
FreydCategoriesForCAP 81.23% <ø> (ø)
GeneralizedMorphismsForCAP 61.60% <ø> (ø)
GradedModulePresentationsForCAP 44.58% <ø> (ø)
GroupRepresentationsForCAP 72.05% <ø> (ø)
HomologicalAlgebraForCAP 73.21% <ø> (ø)
InternalExteriorAlgebraForCAP 93.08% <ø> (ø)
LinearAlgebraForCAP 67.03% <55.02%> (+0.33%) ⬆️
ModulePresentationsForCAP 68.46% <ø> (ø)
ModulesOverLocalRingsForCAP 90.70% <ø> (ø)
MonoidalCategories 89.98% <66.97%> (-0.56%) ⬇️
ToricSheaves 21.79% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mohamed-barakat
mohamed-barakat requested changes Oct 17, 2023
View reviewed changes
Copy link
Member

@mohamed-barakat mohamed-barakat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please

  1. rebase;
  2. add a first commit to bump the version and the dependency of the three packages CAP, FreydCategoriesForCAP and LinearAlgebraForCAP;
  3. add a last commit bumping the version of MonoidalCategories and CartesianCategories.

@mohamed-barakat
Copy link
Member

  1. add a first commit to bump the version and the dependency of the three packages CAP, FreydCategoriesForCAP and LinearAlgebraForCAP;

In the first commit please also bump the dependency on the released MonoidalCategories and CartesianCategories where possible.

zickgraf
zickgraf reviewed Oct 19, 2023
View reviewed changes
MonoidalCategories/gap/BraidedMonoidalCategoriesDerivedMethods.gi Outdated
##TODO: Use BraidingWithGiven
return InverseForMorphisms( cat, Braiding( cat, object_1, object_2 ) );

return BraidingWithGivenTensorProducts( cat, object_2_tensored_object_1, object_2, object_1, object_1_tensored_object_2 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only holds in symmetric categories, and already exists in SymmetricMonoidalCategoriesDerivedMethods.gi.

zickgraf
zickgraf reviewed Oct 19, 2023
View reviewed changes
CartesianCategories/gap/SymmetricCartesianClosedCategoriesDerivedMethods.gi Outdated
@@ -879,7 +879,7 @@ AddDerivationToCAP( CartesianPostComposeMorphismWithGivenObjects,

braiding := CartesianBraiding( cat, ExponentialOnObjects( cat, b, c ), ExponentialOnObjects( cat, a, b ) );

return PreCompose( cat, braiding, CartesianPreComposeMorphism( cat, a, b, c ) );
return PreComposeList( cat, source, [ braiding, CartesianPreComposeMorphism( cat, a, b, c ) ], range );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the desired solution. The source of PreCompose is the source of the first morphism, i.e. the source of braiding. So the desired solution is that braiding has the expected source in the first place.

Thinking more about this, I think we should never use PreComposeList for literal lists. The information about source and range is already available in this case, so adding source and range explicitly does not add anything if everything else is correct. And if things are wrong, adding source and range only hides actual errors. For example, if braiding would have the wrong source here, using PreComposeList would just hide the wrong source from CompilerForCAP.

@mohamed-barakat
Copy link
Member

@TKuh: Could you please make a separate PR out of the first commit? This should fix gap-system/PackageDistro#846.

@TKuh TKuh mentioned this pull request Oct 19, 2023
Merged
@TKuh TKuh force-pushed the WithGivens_monoidal branch 2 times, most recently from 46d99cc to da8624c Compare November 29, 2023 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zickgraf zickgraf zickgraf left review comments

@mohamed-barakat mohamed-barakat mohamed-barakat requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TKuh @mohamed-barakat @zickgraf